reporegistry: do not prepend `repositories` in confPaths by mvo5 · Pull Request #1179 · osbuild/images

mvo5 · 2025-01-29T09:55:47Z

This PR changes the API for reporegistry in two incompatible
ways. The important change is that we want to drop the
prepending of repositories when loading from confPaths.

Doing this on its own has the disadvantage of breaking
the API without breaking the builds. So this includes a
small tweak to allow adding repos via fs.FS directly
which will mean the code in ibcli will become simpler:

diff --git a/cmd/image-builder/repos.go b/cmd/image-builder/repos.go
index bc238dc..5d16a95 100644
--- a/cmd/image-builder/repos.go
+++ b/cmd/image-builder/repos.go
@@ -2,8 +2,6 @@ package main
 
 import (
        "io/fs"
-       "os"
-       "path/filepath"
 
        "github.com/osbuild/images/data/repositories"
        "github.com/osbuild/images/pkg/reporegistry"
@@ -27,17 +25,6 @@ var newRepoRegistry = func(dataDir string) (*reporegistry.RepoRegistry, error) {
                dataDirs = defaultDataDirs
        }
 
-       // XXX: think about sharing this with reporegistry?
-       var fses []fs.FS
-       for _, d := range dataDirs {
-               fses = append(fses, os.DirFS(filepath.Join(d, "repositories")))
-       }
-       fses = append(fses, repos.FS)
-
        // XXX: should we support disabling the build-ins somehow?
-       conf, err := reporegistry.LoadAllRepositoriesFromFS(fses)
-       if err != nil {
-               return nil, err
-       }
-       return reporegistry.NewFromDistrosRepoConfigs(conf), nil
+       return reporegistry.New(dataDirs, []fs.FS{repos.FS})
 }

And for composer it will just be adding a nil to the New()
call.

Note that this also allows us to unexport the LoadAllRepositories
repository loading as this is only used by composer or ibcli.

reporegistry: do not prepend repositories in confPaths

This commit breaks the API by changing the confPaths loading
in reporegistry to stop prepending repositories/. This makes
it easier to use this interface in e.g. cmd/build.

Note that this requires changes in the upstream projecs, notably
osbuild-composer and image-builder-cli.

This commit changes reporegistry.New() to take a new parameter
repoConfigFS []fs.FS. With that we can unexport the public
LoadAllRepositories{,FromFS} as the only user for those is
composer and that can simply use .New().

This commit changes cmd/composer to use the `reporegistry.New` call again. This is a preparation for osbuild/images#1179 And undoes parts of osbuild#4378 but that is no longer necessary because in osbuild/images#946 the error is now passed on from `reporegistry.New()` in the same way as from `LoadRepositories()`.

thozza

The idea of the change looks good to me, but the code needs some fixes for tests to pass...

This commit changes cmd/composer to use the `reporegistry.New` call again. This is a preparation for osbuild/images#1179 And undoes parts of #4378 but that is no longer necessary because in osbuild/images#946 the error is now passed on from `reporegistry.New()` in the same way as from `LoadRepositories()`.

This commit updates the cloudapi test that uses the real repositories to use the version of those from the "images" library. Composer no longer carries the default repos. Note that this can most likely be simplified once osbuild/images#1179 is merged.

schutzbot · 2025-02-06T10:14:10Z

This PR changes the images API or behaviour causing integration failures with osbuild-composer. The next update of the images dependency in osbuild-composer will need work to adapt to these changes.

This is simply a notice. It will not block this PR from being merged.

This commit changes `reporegistry.New()` to take a new parameter `repoConfigFS []fs.FS`. With that we can unexport the public `LoadAllRepositories{,FromFS}` as the only user for those is `composer` and that can simply use `.New()`. Note that we could also change `New()` to just take a single `repoConfig []fs.FS` but the downside of this is that the error reporting becomes more awkward, i.e. to report what paths where configured would (probably) require to build them manually via `fs.FS.Open(".").Stat().Name()` (but I have not tested this).

This commit breaks the API by changing the confPaths loading in reporegistry to stop prepending `repositories/`. This makes it easier to use this interface in e.g. `cmd/build`. Note that this requires changes in the upstream projecs, notably osbuild-composer and image-builder-cli.

This commit updates the cloudapi test that uses the real repositories to use the version of those from the "images" library. Composer no longer carries the default repos. Note that this can most likely be simplified once osbuild/images#1179 is merged.

thozza

This looks OK to me, thanks.